🔍 Adversarial Review — PR #1

Summary

A well-engineered feature with strong documentation and benchmark data. The three-phase pipeline (RDFC-1.0 → WL → rdflib) is architecturally sound but introduces significant complexity. I found 2 bugs (dead code shipped as functional features), 1 algorithmic concern in collision handling, and several design/test gaps worth addressing before merge.

🐛 Bugs & Issues

1. Dead code: normalize_prefixes field and CLI option do nothing

The normalize_prefixes: bool = False field is added to Generator (line ~429) and a --normalize-prefixes/--no-normalize-prefixes CLI option is registered — but self.normalize_prefixes is never read in any generator code in this PR. Users who pass --normalize-prefixes get silent no-op behavior. This should either be removed from this PR (it belongs in PR #4) or wired up.

# Added to Generator dataclass but never checked anywhere:
normalize_prefixes: bool = False

2. Dead code: well_known_prefix_map() defined but never called

The function is defined in generator.py but is not imported or called anywhere in this PR. It returns {str(ns): str(pfx) for pfx, ns in Graph().namespaces() if str(pfx)} — a dynamic map that changes across rdflib versions (see Concern #3). PR #4 redefines this with a static frozen map, creating a direct merge conflict.

3. WL collision counter assignment depends on c14n ordering, not structure

In _wl_signatures(), when two structurally identical blank nodes produce the same WL hash, the counter suffix (_0 vs _1) is assigned by iterating sorted(bnode_ids) — which sorts by canonical c14n ID (e.g., "c14n0" < "c14n42"):

for bid in sorted(bnode_ids):  # sorted by c14n ID, NOT by structure
    digest = hashlib.sha256(sig[bid].encode("utf-8")).hexdigest()[:12]
    count = seen_hashes.get(digest, 0)
    seen_hashes[digest] = count + 1
    label = f"b{digest}" if count == 0 else f"b{digest}_{count}"

Adding an unrelated triple can change RDFC-1.0 numbering, which changes which colliding node gets the base label vs _1 suffix. This defeats WL's core promise of diff-stable IDs in the (admittedly rare) collision case. Fix: use the full WL signature string as a secondary sort key before assigning counters:

for bid in sorted(bnode_ids, key=lambda b: (sig[b], b)):

⚠️ Concerns

1. well_known_prefix_map() is rdflib-version-dependent (direct conflict with PR #4)

Graph().namespaces() returns different prefix sets across rdflib 6.x vs 7.x (e.g., brick, csvw, geo were added/changed). This means "deterministic" output can change on dependency upgrade. PR #4 fixes this with _WELL_KNOWN_PREFIX_MAP: MappingProxyType — a static frozen map. Both PRs also add normalize_prefixes to Generator. These are two direct merge conflicts.

Recommendation: Remove well_known_prefix_map() and normalize_prefixes from this PR entirely. They're unused dead code here and belong exclusively in PR #4.

2. Sorting OWL expression members by repr is fragile

members = sorted(members, key=repr)

repr() for LinkML model objects depends on dataclass field ordering and internal representation. While stable within a single Python version, it can change across:

• Python versions (if dataclass repr format changes)
• linkml-runtime versions (if fields are reordered or types change)
• Any field containing objects with id()-based repr

A more robust approach: define an explicit sort key using stable, semantic fields (e.g., key=lambda x: (x.range or "", x.minimum_value or "")) or serialize to a canonical string.

3. _mutate_kitchen_sink writes temp files to the test input directory

out_path = ks_path.parent / f"_benchmark_mutated_{os.getpid()}_kitchen_sink.yaml"

This writes to tests/linkml/test_generators/input/ which may be read-only in containerized CI. Also, yaml.dump(yaml.safe_load(...)) reformats the entire schema (quoting styles, flow/block, comment stripping), so the benchmark diffs measure YAML reformatting noise in addition to the intended mutation. Consider using tmpdir fixture or string manipulation instead of full YAML round-trip.

4. No RDFC-1.0 timeout for pathological graphs

The W3C RDFC-1.0 spec acknowledges exponential worst-case complexity for certain graph topologies. pyoxigraph.Dataset.canonicalize() has no timeout parameter exposed. A signal.alarm() or thread-based timeout would protect CI from hangs on adversarial input.

5. _deep_sort doesn't propagate parent_key into list items

sorted_items = [_deep_sort(item) for item in value]  # parent_key defaults to ""

If a list item is itself a list, it will be sorted regardless of whether the grandparent key is in _JSONLD_ORDERED_KEYS. Not a practical JSON-LD issue today, but a latent correctness gap.

6. O(n×m) prefix filtering

if pfx_s and any(iri.startswith(ns_s) for iri in used_iris):

With ~30 prefixes × thousands of IRIs, this is quadratic. A trie or pre-sorted binary search would scale better for large ontologies (though the current 68K-triple benchmarks likely absorb this).

7. xsd:string stripping is correct but lossy

The code strips explicit xsd:string datatype annotations during pyoxigraph→rdflib conversion. This is correct per RDF 1.1 §2.5.1 (simple literals = xsd:string), but users who intentionally annotated xsd:string for tooling compatibility lose the annotation after round-trip. Worth documenting this in the docstring.

🧪 Test Coverage Assessment

Strong coverage (✅):

• Byte-level idempotency across runs (4 generators)
• Sorted key verification for JSON generators
• Prefix format (@prefix vs PREFIX)
• Large schema stability (kitchen_sink)
• xfail documentation of non-isomorphism trade-off
• Non-deterministic mode regression guard

Gaps (❌):

• No test for WL hash collisions: Need a graph with two structurally identical blank node subgraphs to verify counter-based dedup produces stable output
• No test for deep blank node nesting (>4 levels): 4 WL iterations may be insufficient for chains of 5+ structurally similar nodes
• No test for well_known_prefix_map(): Dead code, but if kept, needs coverage
• No test for normalize_prefixes: Dead code — the CLI option silently does nothing
• xfail tests document non-isomorphism but don't verify semantic equivalence: The assertion "OWL/SHACL interpret these as unordered sets" should be backed by a test that verifies the same classes/constraints exist in both modes (e.g., compare extracted sh:in value sets, not just triple counts)
• Benchmark YAML round-trip may inflate diff counts: yaml.dump(yaml.safe_load(original)) reformats the entire file — diff measurements may include YAML formatting noise, not just the intended mutation
• Network tests (@pytest.mark.network): schema.org download can fail in CI. No conftest.py marker filtering shown — these may run and flake by default
• Performance test (10s limit): Fragile on slow CI runners; consider a more generous threshold or @pytest.mark.slow marker

📋 Fix Plan

1. Remove normalize_prefixes and well_known_prefix_map() from this PR — they are dead code, belong in PR fix(generators): add --normalize-prefixes flag for well-known prefix names #4, and create merge conflicts
2. Fix WL collision counter ordering: Sort by (sig[bid], bid) instead of just bid to make counter assignment structure-dependent
3. Add a WL collision test: Create a graph with two identical blank node subgraphs, verify both get stable IDs across runs
4. Replace repr sorting with explicit key function for OWL expression members
5. Use tmp_path fixture for _mutate_kitchen_sink output (create a symlink or copy imports alongside)
6. Add @pytest.mark.slow to performance tests and document CI threshold expectations
7. Document xsd:string stripping in deterministic_turtle() docstring explicitly as a known behavior

✅ What's Good

• Excellent documentation: Thorough docstrings with W3C spec references, clear parameter docs, and well-written PR description with benchmark tables
• Lazy import pattern: pyoxigraph is only loaded when --deterministic is used; graceful ImportError with actionable message
• Defensive test design: pytestmark = pytest.mark.skipif(not _has_pyoxigraph, ...) and fixture-level pytest.skip() for network tests
• The hybrid pipeline is clever: RDFC-1.0 for correctness, WL for diff stability, rdflib for Turtle readability — each phase has a clear purpose
• The 94% SHACL size reduction (inline blank nodes + collection syntax) is a compelling result
• xfail tests properly document trade-offs rather than hiding them
• Prefix filtering removes the ~27 rdflib default bindings that leak into output — real quality-of-life improvement